Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase the local sync's ready to flush seconds to 120 #1014

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

beastoin
Copy link
Collaborator

@beastoin beastoin commented Oct 9, 2024

Summary by Entelligence.AI

- Refactor: Enhanced the synchronization progress display to show as an integer percentage value, providing a more intuitive understanding of the sync status.
- Refactor: Increased the local sync's ready to flush seconds to 120, improving the efficiency and reliability of data synchronization.
- New Feature: Introduced a timer control mechanism for better management of synchronization processes, leading to smoother user experience during data syncing.

@beastoin beastoin merged commit f86fbd4 into main Oct 9, 2024
1 check passed
@beastoin beastoin deleted the jzecm_wal_local_sync_958 branch October 9, 2024 12:22
Copy link

github-actions bot commented Oct 9, 2024

Image description Entelligence.AI

Walkthrough

This update enhances the synchronization progress display by showing it as an integer percentage value. It also refactors the timer handling logic in the _LocalSyncWidgetState class, introducing a new boolean to control the timer and modifying its initialization and cancellation based on the sync status. The local sync's ready-to-flush seconds have been increased to 120 for improved performance.

Changes

File(s) Summary
app/lib/pages/memories/sync_page.dart, app/lib/pages/memories/widgets/local_sync.dart The synchronization progress is now displayed as an integer percentage value. The timer handling logic in the _LocalSyncWidgetState class has been refactored with a new boolean controlling the timer and changes to its initialization and cancellation based on the sync status. The local sync's ready-to-flush seconds have been increased to 120.

"In the realm of code where the bits do play,
A change was made, not subtle but bold today.
Syncing progress now clear as day,
And timers dance in a refined ballet.
Ready to flush, we've doubled the delay,
For better performance, so they say. 🎉🚀"


Uplevel your code reviews with Entelligence.AI Pro

Entelligence.AI Pro

If you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between f05038f and 1e934b0 commits.
Files selected (2)
  • app/lib/pages/memories/sync_page.dart (1)
  • app/lib/pages/memories/widgets/local_sync.dart (4)
Review comments generated (5)
  • Review: 4
  • LGTM: 1

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

@@ -76,7 +76,7 @@ class _SyncPageState extends State<SyncPage> {
),
memoryProvider.isSyncing
? Text(
'${memoryProvider.walsSyncedProgress}% synced',
'${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The change from displaying the sync progress as a floating-point number to an integer is a good one for readability. However, it might lead to less precision in the progress display. If the syncing process is slow, users might not see the progress update for a while because the percentage will only increase when it reaches the next whole number. Consider keeping the floating-point precision up to 1 or 2 decimal places.

- '${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced',
+ '${(memoryProvider.walsSyncedProgress * 100).toStringAsFixed(1)}% synced',

Comment on lines +29 to +44
bool _missSecondsInEstTimerEnabled = false;
int _missSeconds = 0;

@override
void initState() {
super.initState();

_missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
if (_missSecondsInEstTimerEnabled) {
setState(() {
_missSeconds++;
});
}
});
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The timer is being initialized in the initState method and it runs every second regardless of whether it's needed or not. This could lead to unnecessary CPU usage. It would be better to start the timer only when it's needed and cancel it when it's not.

-    _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
-      if (_missSecondsInEstTimerEnabled) {
-        setState(() {
-          _missSeconds++;
-        });
-      }
-    });
+    // Move timer initialization to a separate method
+    void startTimer() {
+      _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
+        if (_missSecondsInEstTimerEnabled) {
+          setState(() {
+            _missSeconds++;
+          });
+        }
+      });
+    }

Comment on lines +55 to +59
if (provider.missingWalsInSeconds >= 120) {
_status = LocalSyncStatus.flush;
} else if (!captureProvider.isWalSupported) {
_status = LocalSyncStatus.disabled;
} else if (provider.missingWalsInSeconds > 0) {
} else if (!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The condition for setting _status to LocalSyncStatus.flush has been changed from provider.missingWalsInSeconds > 120 to provider.missingWalsInSeconds >= 120. This change aligns with the PR title, so it seems correct. However, please ensure that this change doesn't introduce any off-by-one errors in your logic.

Comment on lines +75 to +77
_missSecondsInEstTimerEnabled = true;
} else {
_missSecondsInEstTimer?.cancel();
_missSecondsInEstTimer = null;
_missSecondsInEstTimerEnabled = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The timer control logic has been simplified by introducing the _missSecondsInEstTimerEnabled flag. This is a good approach as it makes the code easier to read and understand. However, remember to call startTimer() and cancelTimer() methods appropriately based on the _missSecondsInEstTimerEnabled flag value.

-        _missSecondsInEstTimerEnabled = true;
+        if (!_missSecondsInEstTimerEnabled) {
+          _missSecondsInEstTimerEnabled = true;
+          startTimer();
+        }
-        _missSecondsInEstTimerEnabled = false;
+        if (_missSecondsInEstTimerEnabled) {
+          _missSecondsInEstTimerEnabled = false;
+          cancelTimer();
+        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant